Please address the comments from this code review:

## Overall Comments
- The `locateRunfile` loop now uses `getRunfilePath` but only updates `dir`/`oldDir` on non-`ErrRunfileNotFound` errors, so when `ErrRunfileNotFound` is returned you never advance the directory and can spin in an infinite loop; consider always updating `oldDir`/`dir` before the next iteration.
- `ErrCircularDependency` currently constructs an error with the message `"invalid shell alias"`, which looks like a copy-paste mistake; it should probably have a circular-dependency-specific message.
- In `resolver.RunTask` you call `pipeline.Start(ctx)` once in a goroutine and again synchronously later, which will execute the pipeline twice concurrently; refactor so the pipeline is started exactly once and the watcher just reacts to its lifecycle.

## Individual Comments

### Comment 1
<location> `cmd/run/main.go:219` </location>
<code_context>
-				if err2, ok := err.(*errors.Error); ok {
-					logger.Error(err2.Error(), err2.SlogAttrs()...)
-				}
+			if err := runfile.RunTask(ctx, runfilePath, args[0], kv); err != nil {
+				return err
 			}
</code_context>

<issue_to_address>
**question (bug_risk):** New RunTask call ignores parallel and watch flags that were previously supported

The previous code passed a `RunOption` struct into `rf.Run`, which carried `--parallel`, `--watch`, and other flags. The new `runfile.RunTask` call no longer receives these options, so those flags are now ignored. If this behavior change isn’t intentional, please extend `RunTask` (or add a wrapper) so it accepts and honors the same execution options.
</issue_to_address>

### Comment 2
<location> `pkg/runfile/resolver/env.go:19-24` </location>
<code_context>
+)
+
+func parseDotEnvFilesInto(store map[string]string, files []string) error {
+	for _, f := range files {
+		if !filepath.IsAbs(f) {
+			return errors.New("dotenv file must have absolute paths").KV("dotenv.file", f)
+		}
+
+		f, err := os.Open(f)
+		if err != nil {
+			return errors.New("failed to open dotenv file").Wrap(err).KV("dotenv.file", f)
</code_context>

<issue_to_address>
**issue:** Variable shadowing in parseDotEnvFilesInto makes error metadata misleading

Inside `parseDotEnvFilesInto`, the `f` from `os.Open(f)` shadows the filename:

```go
for _, f := range files {
    if !filepath.IsAbs(f) {
        return errors.New("dotenv file must have absolute paths").KV("dotenv.file", f)
    }

    f, err := os.Open(f)
    if err != nil {
        return errors.New("failed to open dotenv file").Wrap(err).KV("dotenv.file", f)
    }
}
```

At the `KV("dotenv.file", f)` call in the error path, `f` is an `*os.File`, so the metadata won’t contain the path. Renaming the file handle (e.g. `fh`) will preserve the actual filename in the error context.
</issue_to_address>

### Comment 3
<location> `pkg/runfile/resolver/task.go:130-139` </location>
<code_context>
+		return err
+	}
+
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		<-ctx.Done()
+		// ctx.Logger().Info("fwatcher is closing ...")
+		watch.Close()
+	}()
+
+	// executors := []executor.Executor{pipeline}
+	//
+	// if rt.Watch.SSE != nil && rt.Watch.SSE.Addr != "" {
+	// 	executors = append(executors, executor.NewSSEExecutor(executor.SSEExecutorArgs{Addr: rt.Watch.SSE.Addr}))
+	// }
+
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		if err := pipeline.Start(ctx); err != nil {
+			slog.Error("starting command", "err", err)
+		}
+		slog.Debug("final executor start finished")
+	}()
+
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		<-ctx.Done()
+		pipeline.Stop()
+		slog.Debug("2. context cancelled")
+	}()
+
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		watch.Watch(ctx)
+		slog.Debug("3. watcher closed")
+	}()
+
+	pipeline.Start(ctx)
+
+	counter := 0
</code_context>

<issue_to_address>
**issue (bug_risk):** RunTask starts the pipeline twice: once in a goroutine and once synchronously

`Resolver.RunTask` calls `pipeline.Start(ctx)` both in a goroutine and again at the end of the function, so the same pipeline may run concurrently and the second call’s error is dropped. Start the pipeline in exactly one place (either in the goroutine that coordinates with the watcher or synchronously before setting up the watcher) and make sure the returned error is handled in a single, consistent path.
</issue_to_address>

### Comment 4
<location> `cmd/run/completions/run.fish:29` </location>
<code_context>
-complete -c $PROGNAME -l help -s h -d 'show help'
-complete -c $PROGNAME -l list -s l -d 'list all tasks'
-complete -c $PROGNAME -rF -l file -s f -d 'runs targets from this runfile' 
+complete -c completion -n '__fish_completion_no_subcommand' -f -l help -s h -d 'show help'
+complete -c completion -n '__fish_completion_no_subcommand' -f -l help -s h -d 'show help'

</code_context>

<issue_to_address>
**issue (bug_risk):** Fish completion uses `completion` as the command name instead of `run`

Both new `complete` lines target `completion` instead of the actual CLI (`run` or the real binary name), so fish completions likely won’t work as intended. Also, the two lines are identical; please remove the duplicate and point the remaining one at the correct command name.
</issue_to_address>

### Comment 5
<location> `Runfile.yml:43-45` </location>
<code_context>
+        go test -json ./pkg/runfile/resolver/... $pattern_args | gotestfmt $testfmt_args
+        go test -json ./pkg/executor/... $pattern_args | gotestfmt $testfmt_args

   test:only-failing:
     cmd:
-      - go test -json ./pkg/runfile | gotestfmt --hide successful-tests
+      - go test -json ./pkg/runfile/resolver/... | gotestfmt --hide successful-tests
+        go test -json ./pkg/executor/... $pattern_args | gotestfmt --hide successful-tests
+
</code_context>

<issue_to_address>
**issue (bug_risk):** Second go test command in `test:only-failing` is missing a list item marker

In `test:only-failing`, the second `go test` line is missing a leading `-`, so YAML will treat it as part of the first command rather than a separate list item. If you intend to run both commands, it should be:

```yaml
  test:only-failing:
    cmd:
      - go test -json ./pkg/runfile/resolver/... | gotestfmt --hide successful-tests
      - go test -json ./pkg/executor/... $pattern_args | gotestfmt --hide successful-tests
```
</issue_to_address>

### Comment 6
<location> `pkg/runfile/resolver/task_test.go:486-490` </location>
<code_context>
+	}
+}
+
+// Helper to check if a step uses the correct executor type
+func isInteractiveStep(step executor.Step) bool {
+	// This is a basic check - in real tests you might want to
+	// use reflection or add a method to identify command types
+	return len(step.Commands) > 0
+}
</code_context>

<issue_to_address>
**nitpick (testing):** Unused helper `isInteractiveStep` suggests tests might be incomplete for distinguishing interactive vs non-interactive commands

This helper isn’t used and its current implementation (`len(step.Commands) > 0`) doesn’t distinguish interactive vs non‑interactive commands, so it isn’t validating the intended behavior. I recommend either removing it, or wiring it into tests that explicitly assert interactive tasks use `NewInteractiveShellCommand` and non‑interactive ones use `NewShellCommand` (e.g., via a test‑only marker/interface on the command type).
</issue_to_address>

### Comment 7
<location> `pkg/runfile/resolver/env_test.go:11` </location>
<code_context>
+	"testing"
+)
+
+func TestParseDotEnvFilesInto(t *testing.T) {
+	tests := []struct {
+		name           string
</code_context>

<issue_to_address>
**suggestion (testing):** ParseDotEnvFilesInto tests don’t cover invalid dotenv file contents

Current cases cover empty lists, relative paths, missing files, and key overrides, but not the error path from `godotenv.Parse`.

Please add a test that:
- Creates a `.env` file with invalid syntax (e.g., `INVALID LINE WITHOUT EQUALS`),
- Passes its absolute path to `parseDotEnvFilesInto`, and
- Asserts an error is returned, ideally checking that it reflects a parse failure rather than just file I/O.

This will verify behavior when users provide malformed `.env` files.

Suggested implementation:

```golang
import (
	"context"
	"maps"
	"os"
	"path/filepath"
	"strings"
	"testing"
)

```

```golang
	}{
		{
			name:         "when files list is empty, it must pass",
		},
		{
			name: "when dotenv file has invalid syntax, it must return a parse error",
			setupFiles: map[string]string{
				// invalid dotenv syntax: missing '=' separator
				".env": "INVALID LINE WITHOUT EQUALS",
			},
			// This filename will be converted to an absolute path in the test setup;
			// we do not use relative/non-existent path flags so that we exercise
			// the parse error path instead of I/O failures.
			files:        []string{".env"},
			initialStore: map[string]string{},
			// No variables are expected to be loaded on parse failure.
			expected:       map[string]string{},
			wantErr:        true,
			useRelPath:     false,
			useNonExistent: false,
		},

```

To fully satisfy the "ideally checking that it reflects a parse failure" part of your comment, you should also update the test loop in `TestParseDotEnvFilesInto` (the part that calls `parseDotEnvFilesInto`) to special-case this new scenario and assert on the error message, for example:

1. After calling `err := parseDotEnvFilesInto(ctx, store, files...)` and before the generic `wantErr` checks, add logic such as:

```go
if tt.name == "when dotenv file has invalid syntax, it must return a parse error" {
    if err == nil {
        t.Fatalf("expected parse error, got nil")
    }
    if !strings.Contains(err.Error(), "parse") && !strings.Contains(err.Error(), "invalid line") {
        t.Fatalf("expected parse-related error, got %q", err)
    }
}
```

2. Adjust variable names (`err`, `tt`, etc.) to match the existing test body. Ensure this check runs only for this specific test case so other tests remain unchanged.
</issue_to_address>

### Comment 8
<location> `pkg/runfile/resolver/task.go:92` </location>
<code_context>
+	}, nil
+}
+
+func (r *Resolver) RunTask(ctx context.Context, name string) error {
+	rt, err := r.GetTask(name)
+	if err != nil {
</code_context>

<issue_to_address>
**issue (complexity):** Consider refactoring `RunTask`, `createSteps`, and the printing helpers into smaller focused helpers and interfaces to simplify orchestration, recursion, and UI concerns in this resolver.

You can reduce complexity meaningfully here without changing behavior by:

---

### 1. Untangle `RunTask` orchestration and watcher lifecycle

`RunTask` currently:

- Builds steps and pipeline
- Handles watch creation
- Manages multiple goroutines + `WaitGroup`
- Calls `pipeline.Start(ctx)` more than once
- Contains commented‑out experimental code

You can push the concurrency + watch wiring into a helper and keep a single `Start` call:

```go
func (r *Resolver) RunTask(ctx context.Context, name string) error {
	rt, err := r.GetTask(name)
	if err != nil {
		return err
	}

	lw := &writer.LogWriter{Writer: os.Stderr}
	steps, err := r.createSteps(rt, createCommandGroupArgs{
		Stdout: lw,
		Stderr: lw,
	})
	if err != nil {
		return err
	}

	pipeline := executor.NewPipeline(slog.Default(), steps)

	if rt.Watch == nil || !rt.Watch.Enabled {
		return pipeline.Start(ctx)
	}

	return runWithWatch(ctx, rt, pipeline)
}
```

Then put all the goroutine/`WaitGroup` handling into a dedicated helper:

```go
func runWithWatch(ctx context.Context, rt *ResolvedTask, pipeline *executor.Pipeline) error {
	watch, err := watcher.NewWatcher(ctx, watcher.WatcherArgs{
		WatchDirs:            rt.Watch.Dirs,
		IgnoreDirs:           rt.Watch.IgnoreDirs,
		WatchExtensions:      rt.Watch.Extensions,
		IgnoreExtensions:     rt.Watch.IgnoreExtensions,
		IgnoreList:           watcher.DefaultIgnoreList,
		Interactive:          rt.Interactive,
		ShouldLogWatchEvents: false,
	})
	if err != nil {
		return err
	}
	defer watch.Close()

	var wg sync.WaitGroup
	ctx, cancel := context.WithCancel(ctx)
	defer cancel()

	wg.Add(1)
	go func() {
		defer wg.Done()
		_ = pipeline.Start(ctx)
	}()

	wg.Add(1)
	go func() {
		defer wg.Done()
		watch.Watch(ctx)
	}()

	for ev := range watch.GetEvents() {
		slog.Debug("received", "event", ev)
		// existing reload logging
	}

	cancel()
	wg.Wait()
	return nil
}
```

This keeps `RunTask` as a high‑level entry point, with a single pipeline lifecycle.

---

### 2. Split env evaluation + recursion in `createSteps`

`createSteps` mixes:

- Recursive `run` expansion
- Env merging and evaluation
- Logging prefix construction
- Step/command creation

You can first extract env resolution and the recursive expansion into helpers to shorten the core function.

Env helper:

```go
func (r *Resolver) buildEnv(task *ResolvedTask, cmd *Command, baseEnv map[string]string) (map[string]string, error) {
	envStore := fn.MapMerge(r.Env, baseEnv)

	if task.Env != nil {
		if err := parseEnvInto(context.TODO(), envStore, task.Env); err != nil {
			return nil, err
		}
	}
	if cmd.Env != nil {
		if err := parseEnvInto(context.TODO(), envStore, cmd.Env); err != nil {
			return nil, err
		}
	}
	return envStore, nil
}
```

Then keep recursion focused on structure:

```go
func (r *Resolver) createSteps(task *ResolvedTask, args createCommandGroupArgs) ([]executor.Step, error) {
	taskTrail := append(append([]string{}, args.TaskTrail...), task.Name)
	logPrefix := strings.Join(taskTrail, " ≫ ")

	var steps []executor.Step
	for _, cmd := range task.Commands {
		envStore, err := r.buildEnv(task, cmd, args.Env)
		if err != nil {
			return nil, err
		}

		if cmd.IsRunTarget {
			rt, err := r.GetTask(cmd.Text)
			if err != nil {
				return nil, err
			}
			substeps, err := r.createSteps(rt, createCommandGroupArgs{
				Stdout:    args.Stdout,
				Stderr:    args.Stderr,
				Env:       envStore,
				Silent:    task.Silent || rt.Silent,
				TaskTrail: taskTrail,
			})
			if err != nil {
				return nil, err
			}
			steps = append(steps, executor.Step{SubSteps: substeps, Parallel: rt.Parallel})
			continue
		}

		steps = append(steps, r.buildShellStep(task, cmd, envStore, args, logPrefix))
	}

	return steps, nil
}
```

And isolate the pure execution wiring (handler, hooks, interactive) in a small helper:

```go
func (r *Resolver) buildShellStep(
	task *ResolvedTask,
	cmd *Command,
	env map[string]string,
	args createCommandGroupArgs,
	logPrefix string,
) executor.Step {
	step := executor.Step{Parallel: task.Parallel}

	cmdHandler := func(c context.Context) *exec.Cmd {
		return CreateCommand(c, CmdArgs{
			Shell:      task.Shell,
			Env:        fn.ToEnviron(env),
			Cmd:        cmd.Text,
			WorkingDir: task.Dir,
			Stdout:     args.Stdout.WithPrefix(logPrefix),
			Stderr:     args.Stderr.WithPrefix(logPrefix),
		})
	}

	preHook := func(c context.Context) error {
		if task.Silent || task.Interactive {
			return nil
		}
		lang := "bash"
		if len(task.Shell) > 0 {
			lang = task.Shell[0]
		}
		printCommand(args.Stderr, logPrefix, lang, strings.TrimSpace(cmd.Text))
		return nil
	}

	if task.Interactive {
		step.Commands = append(step.Commands, executor.NewInteractiveShellCommand(cmdHandler).AddPreHook(preHook))
	} else {
		step.Commands = append(step.Commands, executor.NewShellCommand(cmdHandler).AddPreHook(preHook))
	}
	return step
}
```

This keeps `createSteps` mostly about traversal and step composition.

---

### 3. Move presentation helpers out of core resolver

`printCommand`, `longestLineLen`, `padString`, and theme detection embed UI into the resolver.

You can keep behavior but move this into a separate file and interface, then inject it:

```go
// ui/command_printer.go
type CommandPrinter interface {
	Print(w *writer.LogWriter, prefix, lang, cmd string)
}
```

Resolver only needs the interface:

```go
type Resolver struct {
	// ...
	cmdPrinter CommandPrinter
}
```

Use in `preHook`:

```go
preHook := func(c context.Context) error {
	if task.Silent || task.Interactive {
		return nil
	}
	r.cmdPrinter.Print(args.Stderr, logPrefix, lang, strings.TrimSpace(cmd.Text))
	return nil
}
```

This shortens `task.go` and decouples execution from the chosen visual style while keeping existing appearance via a concrete implementation in another file/package.

---

These extractions keep all features, but localize orchestration, env/recursion, and UI, making the core resolver and runner logic easier to follow and test.
</issue_to_address>

### Comment 9
<location> `pkg/runfile/resolver/env.go:84` </location>
<code_context>
						lazyEvalMap[k] = exec.CommandContext(ctx, shell[0], append(shell[1:], envEvalScript)...)
</code_context>

<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.

*Source: opengrep*
</issue_to_address>

Please address the comments from this code review:

## Overall Comments
- The new `locateRunfile` loop using `getRunfilePath` will return an empty path with `nil` error as soon as `ErrRunfileNotFound` is hit in the first directory; instead, treat `ErrRunfileNotFound` as a signal to move up a directory and only return when a real path is found or you reach the root.
- The `--list` handling in `main.go` is now effectively disabled (the body is fully commented out), so passing `--list` falls through to normal task execution; consider either removing the flag or restoring some minimal behavior to avoid a confusing no-op.
- In `NewInteractiveShellCommand`, the PTY-based command always binds I/O directly to `os.Stdin`/`os.Stdout`, ignoring the configured writers and losing prefixes/log routing; if possible, thread the provided `Stdout`/`Stderr` through or document that interactive tasks bypass the log writer.

## Individual Comments

### Comment 1
<location> `pkg/runfile/resolver/env_test.go:11` </location>
<code_context>
+	"testing"
+)
+
+func TestParseDotEnvFilesInto(t *testing.T) {
+	tests := []struct {
+		name           string
</code_context>

<issue_to_address>
**suggestion (testing):** Add a test for invalid dotenv file contents to exercise error handling

Current tests cover many scenarios but not an invalid dotenv file (e.g., malformed line like `FOO=bar=baz` or arbitrary junk). Please add a small malformed fixture to assert that `parseDotEnvFilesInto` propagates the parse error from `godotenv.Parse`, so the error path for misconfigured environments is exercised.

Suggested implementation:

```golang
	"testing"
)

```

To exercise the error path for malformed dotenv contents, add a dedicated test that writes an invalid `.env` file and asserts that `parseDotEnvFilesInto` returns an error. Place this new test in `pkg/runfile/resolver/env_test.go`, near the existing `TestParseDotEnvFilesInto` (e.g., immediately after it):

```go
func TestParseDotEnvFilesInto_InvalidDotenvContent(t *testing.T) {
	// Create a temporary directory for the malformed dotenv file.
	dir := t.TempDir()

	// Malformed dotenv content: `godotenv.Parse` treats lines with multiple '='
	// as invalid and returns an error.
	const malformedContent = "FOO=bar=baz\n"

	envPath := filepath.Join(dir, ".env.malformed")
	if err := os.WriteFile(envPath, []byte(malformedContent), 0o600); err != nil {
		t.Fatalf("failed to write malformed dotenv file: %v", err)
	}

	// Use a fresh store to ensure we don't depend on any previously set env.
	store := map[string]string{}

	err := parseDotEnvFilesInto(context.Background(), []string{envPath}, store)

	if err == nil {
		t.Fatalf("expected error for malformed dotenv file, got nil")
	}
}
```

If `parseDotEnvFilesInto` currently swallows errors from `godotenv.Parse`, adjust its implementation so that it returns those errors instead. For example, if it looks roughly like:

```go
func parseDotEnvFilesInto(ctx context.Context, files []string, store map[string]string) error {
	for _, f := range files {
		// ...
		envMap, err := godotenv.Parse(fh)
		if err != nil {
			// previously: continue or log
		}
		// ...
	}
	return nil
}
```

change the error handling to:

```go
		envMap, err := godotenv.Parse(fh)
		if err != nil {
			return fmt.Errorf("parse %s: %w", f, err)
		}
```

so that the new test can assert that malformed dotenv files cause `parseDotEnvFilesInto` to fail.
</issue_to_address>

### Comment 2
<location> `pkg/runfile/resolver/task.go:183` </location>
<code_context>
+	return nil
+}
+
+func parseCommands(commands []any) ([]*Command, error) {
+	result := make([]*Command, 0, len(commands))
+
</code_context>

<issue_to_address>
**issue (complexity):** Consider applying small refactors (direct map inspection, env-building helper, trail helper, and a printer abstraction) to simplify `parseCommands` and `createSteps` without changing behavior.

A few targeted refactors could reduce complexity without changing behavior:

### 1. Avoid JSON round‑trip in `parseCommands`

The marshal/unmarshal of `map[string]any` adds overhead and obscures intent. You can directly inspect the map keys and types:

```go
func parseCommands(commands []any) ([]*Command, error) {
	result := make([]*Command, 0, len(commands))

	for _, command := range commands {
		switch c := command.(type) {
		case string:
			if strings.TrimSpace(c) == "" {
				continue
			}
			result = append(result, &Command{Text: c})

		case map[string]any:
			cmd := &Command{}
			if env, ok := c["env"]; ok {
				if m, ok := env.(map[string]any); ok {
					cmd.Env = m
				} else {
					return nil, errors.New("env must be an object").KV("env", env)
				}
			}

			if run, ok := c["run"]; ok {
				runStr, ok := run.(string)
				if !ok || strings.TrimSpace(runStr) == "" {
					return nil, errors.New("empty or invalid run target").KV("run", run)
				}
				cmd.Text = runStr
				cmd.IsRunTarget = true
			} else if commandVal, ok := c["command"]; ok {
				cmdStr, ok := commandVal.(string)
				if !ok || strings.TrimSpace(cmdStr) == "" {
					return nil, errors.New("empty or invalid command").KV("command", commandVal)
				}
				cmd.Text = cmdStr
			} else if cmdVal, ok := c["cmd"]; ok {
				cmdStr, ok := cmdVal.(string)
				if !ok || strings.TrimSpace(cmdStr) == "" {
					return nil, errors.New("empty or invalid cmd").KV("cmd", cmdVal)
				}
				cmd.Text = cmdStr
			} else {
				return nil, errors.New("either 'run' or 'command'/'cmd' must be specified")
			}

			result = append(result, cmd)

		default:
			return nil, errors.New("invalid command type, must be either a string or an object")
		}
	}

	return result, nil
}
```

This preserves behavior but removes JSON and extra error paths.

---

### 2. Extract env building from `createSteps`

`createSteps` currently merges env in‑line for every command, which contributes to its size and nesting. A small helper keeps the orchestration logic clearer:

```go
func (r *Resolver) buildEnv(base map[string]string, taskEnv, cmdEnv map[string]any) (map[string]string, error) {
	envStore := fn.MapMerge(r.Env, base)

	if taskEnv != nil {
		if err := parseEnvInto(context.TODO(), envStore, taskEnv); err != nil {
			return nil, err
		}
	}

	if cmdEnv != nil {
		if err := parseEnvInto(context.TODO(), envStore, cmdEnv); err != nil {
			return nil, err
		}
	}

	return envStore, nil
}
```

Use it in `createSteps`:

```go
for _, cmd := range task.Commands {
	envStore, err := r.buildEnv(args.Env, task.Env, cmd.Env)
	if err != nil {
		return nil, err
	}

	// existing IsRunTarget / shell command logic...
}
```

---

### 3. Extract task trail extension

The manual slice copy is noisy. Encapsulate into a tiny helper:

```go
func extendTrail(trail []string, name string) []string {
	newTrail := make([]string, len(trail)+1)
	copy(newTrail, trail)
	newTrail[len(trail)] = name
	return newTrail
}
```

Then in `createSteps`:

```go
taskTrail := extendTrail(args.TaskTrail, task.Name)
```

This keeps the core flow in `createSteps` more focused on the execution graph.

---

### 4. Separate printing from step construction

You already use `preHook` for printing. To decouple styling from execution, you can have the hook accept a simple description struct and move chroma/lipgloss into a dedicated printer:

```go
type CommandDesc struct {
	Prefix string
	Lang   string
	Text   string
}

type CommandPrinter interface {
	Print(*writer.LogWriter, CommandDesc)
}
```

Printer implementation (styling kept intact, just moved):

```go
type StyledPrinter struct{}

func (StyledPrinter) Print(w *writer.LogWriter, d CommandDesc) {
	printCommand(w, d.Prefix, d.Lang, d.Text)
}
```

Then wire in `createSteps`:

```go
func (r *Resolver) createSteps(task *ResolvedTask, args createCommandGroupArgs) ([]executor.Step, error) {
	// ...
	printer := StyledPrinter{} // or injected

	preHook := func(c context.Context) error {
		if task.Silent || task.Interactive {
			return nil
		}
		str := strings.TrimSpace(cmd.Text)
		lang := "bash"
		if len(task.Shell) > 0 {
			lang = task.Shell[0]
		}
		printer.Print(args.Stderr, CommandDesc{
			Prefix: logPrefix,
			Lang:   lang,
			Text:   str,
		})
		return nil
	}
	// ...
}
```

The behavior stays the same, but `createSteps` no longer needs to know about chroma or lipgloss details, making the orchestration logic easier to read and maintain.
</issue_to_address>

### Comment 3
<location> `pkg/runfile/resolver/env.go:44` </location>
<code_context>
+	return nil
+}
+
+func parseEnvInto(ctx context.Context, envStore map[string]string, envMap map[string]any) error {
+	lookupEnv := func(key string) (string, bool) {
+		if v, ok := os.LookupEnv(key); ok {
</code_context>

<issue_to_address>
**issue (complexity):** Consider refactoring `parseEnvInto` by introducing an explicit env expression type and helper functions to separate parsing from evaluation and decouple from `exec.Cmd`.

You can simplify this without losing any functionality by (1) separating parsing from evaluation, (2) avoiding `map[string]any` “protocol” inside the loop, and (3) decoupling from `*exec.Cmd`.

### 1. Replace `lazyEvalMap` with a small expression type

Instead of `map[string]*exec.Cmd`, collect expressions in a dedicated slice. This keeps the env-building loop simple and removes the tight coupling to `exec.Cmd`:

```go
type envExpr struct {
	Key    string
	Shell  []string
	Script string
}

func parseEnvInto(ctx context.Context, envStore map[string]string, envMap map[string]any) error {
	lookupEnv := func(key string) (string, bool) {
		if v, ok := os.LookupEnv(key); ok {
			return v, true
		}
		if v, ok := envStore[key]; ok {
			return v, true
		}
		return "", false
	}

	var exprs []envExpr

	for k, v := range envMap {
		if _, ok := lookupEnv(k); ok {
			continue
		}

		switch value := v.(type) {
		case string:
			envStore[k] = value

		case map[string]any:
			expr, isExpr, err := parseEnvSpec(k, value)
			if err != nil {
				return err
			}
			if isExpr {
				exprs = append(exprs, expr)
			}

		default:
			envStore[k] = fmt.Sprint(v)
		}
	}

	if err := evalEnvExpressions(ctx, envStore, exprs); err != nil {
		return err
	}

	return nil
}
```

### 2. Make the map semantics explicit with a helper

Instead of scanning `map[string]any` inline with magic keys, pull that into a dedicated parser. This keeps all the error handling and key logic in one place:

```go
var shellAliasMap = map[string][]string{
	"bash": {"bash", "-c"},
	"sh":   {"sh", "-c"},
	// ...
}

func parseEnvSpec(key string, m map[string]any) (envExpr, bool, error) {
	if requiredVal, ok := m["required"]; ok {
		isRequired, ok := requiredVal.(bool)
		if !ok {
			return envExpr{}, false,
				errors.New("ENV-EXPRESSION: value field `required` must be a boolean").
					KV("env.key", key, "env.value", m)
		}
		if isRequired {
			return envExpr{}, false,
				errors.New(fmt.Sprintf("ENV-EXPRESSION: env var '%s' is required, it must be provided", key)).
					KV("env.key", key, "env.value", m)
		}
	}

	for optKey, optVal := range m {
		if shell, ok := shellAliasMap[optKey]; ok {
			script, ok := optVal.(string)
			if !ok {
				return envExpr{}, false,
					errors.New(fmt.Sprintf("ENV-EXPRESSION: value field `%s`, must have a string value", optKey)).
						KV("env.key", key, "env.value", m)
			}
			return envExpr{
				Key:    key,
				Shell:  shell,
				Script: script,
			}, true, nil
		}
	}

	// no expression fields: treat as no-op for now
	return envExpr{}, false, nil
}
```

This preserves the existing behavior (`required`, shell aliases, error messages) but removes the need for type switches and key scanning inside `parseEnvInto`.

### 3. Isolate evaluation logic and reuse `envStore` only via `ToEnviron`

Move the `exec.Cmd` wiring and command execution into a dedicated helper that accepts your explicit env expression type:

```go
func evalEnvExpressions(ctx context.Context, envStore map[string]string, exprs []envExpr) error {
	if len(exprs) == 0 {
		return nil
	}

	cmdEnv := fn.ToEnviron(envStore)

	for _, e := range exprs {
		cmd := exec.CommandContext(ctx, e.Shell[0], append(e.Shell[1:], e.Script)...)
		cmd.Env = cmdEnv

		stdout := new(bytes.Buffer)
		stderr := new(bytes.Buffer)
		cmd.Stdout = stdout
		cmd.Stderr = stderr

		if err := cmd.Run(); err != nil {
			return errors.New("ENV-EXPRESSION: evaluation failed").
				Wrap(err).
				KV("env.key", e.Key, "eval.cmd", cmd.String(), "eval.stderr", stderr.String())
		}

		envStore[e.Key] = strings.TrimSpace(stdout.String())
	}

	return nil
}
```

This keeps all command-related complexity in one place and makes `parseEnvInto` mostly about mapping input declarations to either direct values or `envExpr` entries.

These changes keep all current features (dotenv, required flags, shell expressions, lazy evaluation using the fully-populated env), while making the main function smaller and more declarative.
</issue_to_address>

### Comment 4
<location> `pkg/executor/command.go:12` </location>
<code_context>
+	Run(ctx context.Context) error
+}
+
+type command struct {
+	run       func(ctx context.Context) error
+	preHooks  []func(ctx context.Context) error
</code_context>

<issue_to_address>
**issue (complexity):** Consider replacing the concrete command struct and hook slices with a composable CommandFunc function type and small wrapper helpers to keep hooks while simplifying the abstraction.

The current `command` struct + interface + hook slices add indirection that isn’t currently buying you anything. You can keep pre/post‑hook behavior and extensibility while simplifying the model around a single function type.

A minimal alternative that preserves all functionality:

```go
type Command interface {
	Run(ctx context.Context) error
}

type CommandFunc func(context.Context) error

func (f CommandFunc) Run(ctx context.Context) error {
	return f(ctx)
}

func WithPreHook(cmd CommandFunc, hook CommandFunc) CommandFunc {
	return func(ctx context.Context) error {
		if err := hook(ctx); err != nil {
			return err
		}
		return cmd(ctx)
	}
}

func WithPostHook(cmd CommandFunc, hook CommandFunc) CommandFunc {
	return func(ctx context.Context) error {
		if err := cmd(ctx); err != nil {
			return err
		}
		return hook(ctx)
	}
}
```

Usage stays simple and composable, without the extra struct state and slice management:

```go
base := CommandFunc(func(ctx context.Context) error {
	// core logic
	return nil
})

cmd := WithPreHook(
	WithPostHook(base, postHook),
	preHook,
)

// still satisfies Command
err := cmd.Run(ctx)
```

This keeps:

- A `Command` abstraction (via the interface and `CommandFunc` adapter).
- Support for pre and post hooks (via composition).
- Extensibility (you can add more helpers like `WithTimeout`, `WithMetrics`, etc.).

But it removes the need to track `preHooks`/`postHooks` slices and a concrete `command` struct, reducing complexity when reading and debugging the pipeline.
</issue_to_address>

### Comment 5
<location> `pkg/executor/pipeline.go:59` </location>
<code_context>
+	return nil
+}
+
+func (p *Pipeline) execStep(ctx context.Context, step *Step) error {
+	if err := p.execSubSteps(ctx, step); err != nil {
+		return err
</code_context>

<issue_to_address>
**issue (complexity):** Consider making `execStep` explicitly enforce or branch on the mutually exclusive use of `SubSteps` versus `Commands` so the Step’s role and control flow are easier to understand.

You can reduce the cognitive load without changing the public shape of `Step` by making the “either SubSteps or Commands” rule explicit in `execStep`, instead of always running both paths.

Right now, `execStep` silently runs *both* `SubSteps` and `Commands` if someone populates both, which contradicts the comment and makes the control flow harder to reason about.

You can encode the intended invariant and simplify the execution logic like this:

```go
func (p *Pipeline) execStep(ctx context.Context, step *Step) error {
	hasSub := len(step.SubSteps) > 0
	hasCmd := len(step.Commands) > 0

	switch {
	case hasSub && hasCmd:
		// Enforce the invariant documented on Step.
		// You can also choose to log and run one branch if you prefer.
		return fmt.Errorf("invalid Step: cannot have both SubSteps and Commands")

	case hasSub:
		return p.execSubSteps(ctx, step)

	case hasCmd:
		return p.execCommands(ctx, step)

	default:
		// Empty step is a no-op.
		return nil
	}
}
```

If you don’t want to fail hard, you can still make control flow explicit while preserving the current “run both” behavior, which is easier to read than the current unconditional calls:

```go
func (p *Pipeline) execStep(ctx context.Context, step *Step) error {
	if len(step.SubSteps) > 0 {
		if err := p.execSubSteps(ctx, step); err != nil {
			return err
		}
	}
	if len(step.Commands) > 0 {
		if err := p.execCommands(ctx, step); err != nil {
			return err
		}
	}
	return nil
}
```

Either version makes the Step roles much clearer and directly reflects the intended model, reducing the “single struct doing two roles” confusion without changing the overall pipeline abstraction.
</issue_to_address>

### Comment 6
<location> `pkg/runfile/resolver/env.go:87` </location>
<code_context>
						lazyEvalMap[k] = exec.CommandContext(ctx, shell[0], append(shell[1:], envEvalScript)...)
</code_context>

<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.

*Source: opengrep*
</issue_to_address>

Please address the comments from this code review:

## Overall Comments
- In `parseEnvInto`, the `required` flag currently triggers an error unconditionally and doesn’t check whether the variable is already present via `lookupEnv`; this should only error when the key is actually missing so required variables coming from OS/envStore pass.
- In `resolver.Load`, when normalizing `task.DotEnv` for included Runfiles you use `runfileDir` instead of the included file’s directory (`includedDir`), which will resolve dotenv paths relative to the importing Runfile rather than the included one; consider switching to `includedDir` there.
- The `resolver.Error` type in `resolver/errors.go` wraps `errors.Error` but isn’t used anywhere in the new resolver code; either remove it or start using it consistently for resolver-specific errors to avoid dead code.

## Individual Comments

### Comment 1
<location> `cmd/run/main.go:244-250` </location>
<code_context>
+			continue
+		}
+
+		if stat.IsDir() {
+			return "", fmt.Errorf("Runfile.yml is a directory")
+		}
+
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Error message in `getRunfilePath` hardcodes `Runfile.yml` even when another candidate filename is used.

For directory matches, the error always says `Runfile.yml is a directory`, even when the matched file is `Runfile` or `Runfile.yaml`. Use the actual matched filename (e.g. `fmt.Errorf("%s is a directory", f)`) or the full path instead.

```suggestion
		stat, err := os.Stat(filepath.Join(dir, f))
		if err != nil {
			if !os.IsNotExist(err) {
				return "", err
			}
			continue
		}

		if stat.IsDir() {
			return "", fmt.Errorf("%s is a directory", filepath.Join(dir, f))
		}
```
</issue_to_address>

### Comment 2
<location> `pkg/runfile/resolver/task.go:1` </location>
<code_context>
+package resolver
+
+import (
</code_context>

<issue_to_address>
**issue (complexity):** Consider decomposing this large task.go file by extracting UI/printing helpers, generic command creation, and watcher orchestration into dedicated helpers/files to keep task resolution logic focused and easier to follow.

You can keep the current behavior intact while shaving off a lot of cognitive load by extracting a few clearly separable concerns into small helpers / files.

### 1. Extract UI / printing helpers out of this file

`isDarkTheme`, `printCommand`, `padString`, `longestLineLen` are UI-only and couple this file to termenv, lipgloss, chroma, term. You can move them into a `ui.go` (or `display.go`) and only expose a narrow interface:

```go
// ui.go
package resolver

import (
    "bytes"
    "fmt"
    "os"
    "strings"
    "sync"

    "github.com/alecthomas/chroma/v2/quick"
    "github.com/charmbracelet/lipgloss"
    "github.com/muesli/termenv"
    "github.com/nxtcoder17/runfile/pkg/writer"
    "golang.org/x/term"
)

var (
    darkThemeOnce   sync.Once
    darkThemeResult bool
)

func isDarkTheme() bool {
    darkThemeOnce.Do(func() {
        darkThemeResult = termenv.NewOutput(os.Stdout).HasDarkBackground()
    })
    return darkThemeResult
}

func PrintCommand(w *writer.LogWriter, prefix, lang, cmd string) {
    borderColor := "#4388cc"
    if !isDarkTheme() {
        borderColor = "#3d5485"
    }

    myBorder := lipgloss.Border{
        Top:         "-+",
        Bottom:      "-+",
        Left:        "|",
        Right:       "|",
        TopLeft:     "+",
        TopRight:    "+",
        BottomLeft:  "+",
        BottomRight: "+",
    }

    s := lipgloss.NewStyle().
        Border(myBorder).
        BorderForeground(lipgloss.Color(borderColor)).
        PaddingLeft(1).
        PaddingRight(1)
    defer s.UnsetBorderStyle()
    defer s.UnsetPadding()

    width := 0
    if term.IsTerminal(0) {
        width, _, _ = term.GetSize(0)
    }

    hlCode := new(bytes.Buffer)
    colorscheme := "catppuccin-macchiato"
    if !isDarkTheme() {
        colorscheme = "xcode"
    }

    cmdStr := strings.TrimSpace(cmd)
    longestLen := longestLineLen(cmdStr) + len(prefix) + 2
    if err := quick.Highlight(hlCode, cmdStr, lang, "terminal16m", colorscheme); err != nil {
        fmt.Fprintf(w, "%s %s\n", writer.GetStyledPrefix(prefix), cmdStr)
        return
    }

    if width > 0 && longestLen >= width-2 {
        s = s.Width(width - 2)
    }

    fmt.Fprintf(w, "\r\033[K%s%s\n", padString(s.Render(hlCode.String()), prefix), s.UnsetBorderStyle())
}

func longestLineLen(str string) int {
    sp := strings.Split(str, "\n")
    l := len(sp[0])
    for i := 1; i < len(sp); i++ {
        if len(sp[i]) > l {
            l = len(sp[i])
        }
    }
    return l
}

func padString(str string, withPrefix string) string {
    sp := strings.Split(str, "\n")
    for i := range sp {
        if i == 0 {
            sp[i] = fmt.Sprintf("%s %s", writer.GetStyledPrefix(withPrefix), sp[i])
            continue
        }
        sp[i] = fmt.Sprintf("%s %s", strings.Repeat(" ", len(withPrefix)+2), sp[i])
    }
    return strings.Join(sp, "\n")
}
```

Then in `task.go` you just call:

```go
preHook := func(c context.Context) error {
    if task.Silent || task.Interactive {
        return nil
    }
    str := strings.TrimSpace(cmd.Text)

    lang := "bash"
    if len(task.Shell) > 0 {
        lang = task.Shell[0]
    }
    PrintCommand(args.Stderr, logPrefix, lang, str)
    return nil
}
```

This keeps `createSteps` focused on execution structure instead of styling details.

### 2. Move `CreateCommand` to a `command.go` closer to execution concerns

`CreateCommand` is generic and not task-specific. Pull it into its own file so `task.go` doesn’t mix resolution + UI + low-level command wiring:

```go
// command.go
package resolver

import (
    "context"
    "io"
    "os"
    "os/exec"
)

type CmdArgs struct {
    Shell      []string
    Env        []string // [key=value,...]
    WorkingDir string

    Cmd string

    IsInteractive bool
    Stdout        io.Writer
    Stderr        io.Writer
}

func CreateCommand(ctx context.Context, args CmdArgs) *exec.Cmd {
    shell := args.Shell[0]
    cargs := append(args.Shell[1:], args.Cmd)

    // #nosec G204
    c := exec.CommandContext(ctx, shell, cargs...)
    c.Dir = args.WorkingDir
    c.Env = args.Env
    c.Stdout = args.Stdout
    c.Stderr = args.Stderr

    if args.IsInteractive {
        c.Stdin = os.Stdin
    }

    return c
}
```

Call site stays the same except for exported field name:

```go
cmdHandler := func(c context.Context) *exec.Cmd {
    return CreateCommand(c, CmdArgs{
        Shell:         task.Shell,
        Env:           fn.ToEnviron(envStore),
        Cmd:           cmd.Text,
        WorkingDir:    task.Dir,
        Stdout:        args.Stdout.WithPrefix(logPrefix),
        Stderr:        args.Stderr.WithPrefix(logPrefix),
        IsInteractive: task.Interactive,
    })
}
```

This lets you evolve command execution independently of task resolution/UI.

### 3. Split `RunTask` into orchestration helpers

`RunTask` currently wires everything (resolution, pipeline build, watcher, goroutines). A small decomposition will make it easier to follow and test without changing behavior:

```go
func (r *Resolver) RunTask(ctx context.Context, name string) error {
    rt, err := r.GetTask(name)
    if err != nil {
        return err
    }

    lw := &writer.LogWriter{Writer: os.Stderr}
    pipeline, err := r.buildPipeline(rt, lw)
    if err != nil {
        return err
    }

    if rt.Watch == nil || !rt.Watch.Enabled {
        return pipeline.Start(ctx)
    }

    return r.runWithWatcher(ctx, rt, pipeline)
}

func (r *Resolver) buildPipeline(rt *ResolvedTask, lw *writer.LogWriter) (*executor.Pipeline, error) {
    steps, err := r.createSteps(rt, createCommandGroupArgs{
        Stdout: lw,
        Stderr: lw,
    })
    if err != nil {
        return nil, err
    }
    return executor.NewPipeline(slog.Default(), steps), nil
}

func (r *Resolver) runWithWatcher(ctx context.Context, rt *ResolvedTask, pipeline *executor.Pipeline) error {
    var wg sync.WaitGroup
    watch, err := watcher.NewWatcher(ctx, watcher.WatcherArgs{
        WatchDirs:            rt.Watch.Dirs,
        IgnoreDirs:           rt.Watch.IgnoreDirs,
        WatchExtensions:      rt.Watch.Extensions,
        IgnoreExtensions:     rt.Watch.IgnoreExtensions,
        IgnoreList:           watcher.DefaultIgnoreList,
        Interactive:          rt.Interactive,
        ShouldLogWatchEvents: false,
    })
    if err != nil {
        return err
    }

    wg.Add(1)
    go func() {
        defer wg.Done()
        <-ctx.Done()
        watch.Close()
    }()

    wg.Add(1)
    go func() {
        defer wg.Done()
        if err := pipeline.Start(ctx); err != nil {
            slog.Error("starting command", "err", err)
        }
        slog.Debug("final executor start finished")
    }()

    wg.Add(1)
    go func() {
        defer wg.Done()
        <-ctx.Done()
        pipeline.Stop()
        slog.Debug("2. context cancelled")
    }()

    wg.Add(1)
    go func() {
        defer wg.Done()
        watch.Watch(ctx)
        slog.Debug("3. watcher closed")
    }()

    counter := 0
    for ev := range watch.GetEvents() {
        slog.Debug("received", "event", ev)
        counter++
        slog.Info(fmt.Sprintf("[RELOADING (%d)] due changes in %s", counter, ev.Name))
    }

    wg.Wait()
    return nil
}
```

This keeps `RunTask` at a high-level “resolve → build → run/watch” flow and makes the watcher logic testable in isolation.

These extractions don’t change behavior but reduce the responsibilities of `task.go`, improving cohesion and making future changes (e.g. different UI or execution strategy) easier to implement.
</issue_to_address>

### Comment 3
<location> `pkg/runfile/resolver/env.go:90` </location>
<code_context>
					lazyEvalMap[k] = exec.CommandContext(ctx, shell[0], append(shell[1:], envEvalScript)...)
</code_context>

<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.

*Source: opengrep*
</issue_to_address>